Graphics tests docs #2185
Graphics tests docs #2185
Conversation
|
I haven't been following this closely but it seems like an interesting tactic for handling image tests across dependency versions and platforms. The one problem is that the hashes of 2 very similar images will be very different, meaning we need exact matches to one of our stored hashes. This seems OK as long as we can generate enough hashes, but I wonder how well this will scale in the future. It might be valuable to consider using a different hashing algorithm, one where similar images generate hashes that are "close". Luckily such things exist already, called perceptual hashes. A quick search suggests Python implementations of pHash are available (e.g. https://github.com/JohannesBuchner/imagehash). This would allow storing hashes for major changes (e.g. when matplotlib changes something like default colormap) but minor variants around these could be matched by checking the distance between two hashes rather than having to add new reference hashes for each case. Perhaps worth considering. |
|
Thanks @ajdawson! Perceptual image hashing has been on our radar and already discussed. See here for an initial investigation. It raises the interesting question of how "similar" is a result to an expected result, and is that definition of "similarity" acceptable to suit our testing needs. We thought that we'd first exercise the use of a zero tolerance cryptographic hash approach and assess the impact and practicalities of demanding exact image equivalence rather than "similar" images based on a hamming distance hash threshold. It's certainly a very interesting problem space. With the new work flow pattern and framework that we now have in place for image testing, I think we're in a much healthier place i.e. it's an enabler that's got us pretty darn close to unpinning mpl within Travis from 1.3 to latest mpl 1.5+, which in turn releases us from the numpy 1.8 shackles, see #2124 . So I believe perceptual image hashing is still a viable option that we can chose to use and it's a question that I think we should consider answering sooner rather than later ... |
indeed |
corinnebosley
left a comment
corinnebosley
left a comment
There was a problem hiding this comment.
Really like the changes to the Iris Check List.
The expansion of the steps you have written for the Basic Workflow should fall easily out of the stubs you have already written.
| 'legacy' style tests (as described in :ref:`developer_tests`). | ||
| It is conceivable that new 'graphics tests' of this sort can still be added. | ||
| However, as graphics tests are inherently "integration" style rather than true | ||
| unit tests, results are dependent on the installed versions of dependences (see |
There was a problem hiding this comment.
For consistency, I think this should be 'installed versions of dependencies' rather than '...dependences'.
Maybe for easier reading you could change it to something like 'results vary with installed versions of dependencies' (although this is almost the exact wording of another phrase below, so it would be silly to have it twice).
| ========================= | ||
|
|
||
| Prior to Iris 1.10, all graphics tests compared against a stored reference | ||
| image with a small allowed RMS tolerance, based on RGB values in each pixel. |
There was a problem hiding this comment.
I feel like this sentence might need a bit of clarity or fragmentation or something, it's just a little bit cumbersome. It's concise, which I like, but I had to read it a few times to make it sound right in my head.
| image with a small allowed RMS tolerance, based on RGB values in each pixel. | ||
|
|
||
| From Iris v1.11 onward, we want to support testing Iris against multiple | ||
| versions of matplotlib (and some other dependencies). |
There was a problem hiding this comment.
At some point in this document I think that the word 'dependencies' should be expanded or explained. Maybe just replaced with 'dependent libraries'. Although it is obvious to us what it means, it may not be as clear to a developer looking at this document without the context that we now have.
| database in the repo (which is actually stored in | ||
| ``lib/iris/tests/results/imagerepo.json``). | ||
| * storing associated reference images for each hash value in a separate web | ||
| project, currently in https://github.com/SciTools/test-images-scitools , |
There was a problem hiding this comment.
'web project' feels a bit confusing to me. Maybe just 'repository'?
| Basic workflow | ||
| ============== | ||
| #. a graphics test result has changed, following changes in Iris or | ||
| dependencies, so a test is failing |
There was a problem hiding this comment.
I like this point. It's important to include a trigger or reason for having to do this.
| #. a pull request is created in the Iris repository, including the change to | ||
| the image results database (``tests/results/imagerepo.json``) : | ||
| This pull request must contain a reference to the matching one in | ||
| test-images-scitools. |
There was a problem hiding this comment.
Good stubs. I agree that enumerating the steps will make it easier to add the necessary detail.
| * If new files are required by tests or code examples, they must be added to | ||
| the appropriate supporting project via a suitable pull-request. | ||
| This new 'supporting pull request' should be referenced in the main Iris | ||
| pull request, and must be accepted and merged before the Iris one can be. |
There was a problem hiding this comment.
I really like your additions to this document. Very clear and instructive.
| Certain Iris tests rely on testing plotted results. | ||
| This is required for testing the modules :mod:`iris.plot` and | ||
| :mod:`iris.quickplot`, but is also used for some other legacy and integration | ||
| tests. |
There was a problem hiding this comment.
There are lots of tests in this sentence. Are there too many?
|
@pp-mo and @corinnebosley If #2192 is merged, then it may impact this PR. |
|
#2192 is now merged and this branch has inherited a conflict @pp-mo please may you attempt to capture these inputs and hook together @corinnebosley 's comments and update the PR thank you |
|
@pp-mo I did make a branch from this branch and made some changes to the docs in your absence, but I had trouble working out how to make a PR and where to push it to and stuff, so I stopped. |
Those suggestions are here : pp-mo#39 |
pp-mo
force-pushed
the
graphics_tests_docs
branch
from
0350979 to
b3c3740
Compare
|
but 😄 have merged in all text from @corinnebosley 's above PR |
pp-mo
force-pushed
the
graphics_tests_docs
branch
from
b35eb38 to
e273041
Compare
|
Rebased + resolved. |
|
this is looking pretty close to good to go to me and remaining concerns or objections in short order please... |
|
thanks @pp-mo |
5 participants
First cut at documenting new graphics test arrangements + workflow.
NOT yet complete